[fix](auth) Add missing privilege checks for several Nereids commands - #66218
Open
CalvinKirs wants to merge 4 commits into
Open
[fix](auth) Add missing privilege checks for several Nereids commands#66218CalvinKirs wants to merge 4 commits into
CalvinKirs wants to merge 4 commits into
Conversation
Several Nereids commands do not check privileges before executing. The Nereids path in StmtExecutor dispatches straight to Command.run(), so every command has to enforce its own privileges, and these ones were missed: - AdminSetEncryptionRootKeyCommand / AdminRotateTdeRootKeyCommand: now require ADMIN, like the other ADMIN SET statements. - DropCatalogRecycleBinCommand: now requires ADMIN. It takes a raw object id and erases instantly, so it cannot be authorized at db/table level. - CreateDictionaryCommand: requires CREATE on the target database plus SELECT on the source table, since the load task runs internally. DropDictionaryCommand requires DROP on the database. - AddConstraintCommand / DropConstraintCommand: require ALTER on the table. For a foreign key the referenced table is checked as well, because it gets a reverse reference registered on it. In DropConstraintCommand the check is placed after the name-based fallback so neither resolution path can skip it. - WarmUpClusterCommand / CancelWarmUpJobCommand / DropStageCommand: require ADMIN, matching CreateStageCommand which already did. Add auth_call regression cases for constraint, dictionary and recycle bin covering both the denied and the granted path.
CalvinKirs
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
July 29, 2026 06:52
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Member
Author
|
/review |
…ions Follow-up on the privilege checks added in the previous commit. Dictionary: use checkTblPriv on the dictionary itself instead of checkDbPriv on its database, matching CREATE/DROP TABLE and CREATE/DROP MTMV. checkDbPriv would reject a user who was granted the privilege on the dictionary name directly. Warm up: WARM UP CLUSTER now requires USAGE on the source and destination compute groups instead of global ADMIN, matching UseCloudClusterCommand, plus SELECT on each table named by WITH TABLE. CANCEL WARM UP JOB keeps ADMIN because CloudWarmUpJob records no owner, so a job cannot be scoped to the user who created it.
…intCommand WarmUpClusterOnTablesParseTest builds a bare ConnectContext with no user identity, so the privilege check added to WarmUpClusterCommand.validate() hit a NullPointerException in getRolesByUserWithLdap(). Neither checkGlobalPriv(ctx, ...) nor checkCloudPriv(ctx, ...) tolerates a null identity, which is fine for the real execution path since StmtExecutor always runs with an authenticated context. Give the test an identity and let it bypass the checks, since it covers ON TABLES parsing rather than authorization. Also make AddConstraintCommand use the ConnectContext passed to run() instead of the static ConnectContext.get(). DropConstraintCommand already did, and the static one is null on any path that has no thread local set.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29006 ms |
Contributor
TPC-DS: Total hot run time: 177551 ms |
Contributor
ClickBench: Total hot run time: 25.03 s |
Contributor
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 29290 ms |
Contributor
TPC-DS: Total hot run time: 177166 ms |
Contributor
ClickBench: Total hot run time: 24.94 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Blocking authorization gaps remain in the constraint, dictionary, and cloud warm-up changes; the six inline comments contain the concrete reproductions and fixes.
Checkpoint conclusions:
- Goal and effective privileges: incomplete. The intended predicates are ALTER on constraint endpoints, CREATE/DROP for dictionaries plus source SELECT, global ADMIN for the administrative commands, and compute-group USAGE plus table SELECT where applicable. The accepted findings show cross-table mutation, pre-authorization metadata disclosure, row/data-policy bypass, and incomplete warm-up table authorization.
- Scope and parallel paths: the diff is small, but the review traced constraint cascades/replay, dictionary creation/load/refresh/read, all warm-up syntax modes and job lifecycle, forwarding, privilege inheritance, and the changed tests. No extra
review_focus.txtguidance was supplied, so the whole PR was reviewed. - Correctness, concurrency, lifecycle, and persistence: constraint metadata changes remain manager-locked and replay/MTMV invalidation are equivalent; dictionary loads and warm-up refreshes run asynchronously, and their missing caller identity is captured by the accepted policy/recheck findings. Warm-up journaling and forwarded context installation are otherwise consistent, with no separate lock, deadlock, atomicity, crash, or failover defect found.
- Configuration and compatibility: no config, FE-BE variable/contract, function-symbol, storage-format, or rolling-upgrade change is introduced.
- Performance and observability: the added privilege lookups are not a material hot-path cost. Existing logs identify the affected operations; no separate metrics gap was found, while unauthorized matched-table visibility is part of the accepted
ON TABLESissue. - Tests: the added suites cover only UNIQUE constraints, basic dictionary grants, and recycle-bin ADMIN, while the warm-up unit test explicitly bypasses auth. They do not cover FK cascades, metadata-order denial, row/mask policies, warm-up catalog mismatch, dynamic matches, or forwarding. Per the review contract, no local build or tests were run.
| TableNameInfo tableNameInfo = TableNameInfoUtils.fromCatalogDb( | ||
| table.getDatabase().getCatalog(), table.getDatabase(), table); | ||
| ImmutableList<String> columns = columnsAndTable.first; | ||
| checkAlterPriv(ctx, tableNameInfo); |
Contributor
There was a problem hiding this comment.
The relation and columns have already been bound by extractColumnsAndTable() before this ALTER gate; ANALYZED_PLAN returns before the rewrite-stage privilege rule. Consequently a caller without ALTER can distinguish missing tables/columns from valid protected ones, and the FK path similarly probes the referenced side before its second check. Normalize both table names and authorize them first, then bind columns; add no-grant existing/missing-object tests plus an ALTER-without-SELECT success case.
| throw new AnalysisException("Failed to create dictionary: " + e.getMessage()); | ||
| } | ||
|
|
||
| // 2. Check auth. Must run after validateAndSet(), which fills in the default catalog/db names. |
Contributor
There was a problem hiding this comment.
validateAndSet() has already resolved the source catalog/DB/table, read its full schema, and validated column names/types before either new check runs. A user without SELECT can therefore distinguish missing objects/columns and invalid types from the eventual access denial. Split default-name normalization from metadata binding, run the target/source checks on the normalized names, and only then resolve and validate the source.
| String srcCtl = createDictionaryInfo.getSourceCtlName(); | ||
| String srcDb = createDictionaryInfo.getSourceDbName(); | ||
| String srcTbl = createDictionaryInfo.getSourceTableName(); | ||
| if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx, srcCtl, srcDb, srcTbl, |
Contributor
There was a problem hiding this comment.
A plain table-level SELECT check does not establish that the creator may read the raw source rows. createDictionary() schedules dataLoad(null, ...), which builds an ADMIN context; LogicalCheckPolicy skips row filters and data masks for ADMIN, so a policy-restricted user can create a dictionary containing rows/unmasked values their own SELECT would hide and read them with dict_get. Preserve the effective principal/policies for the load (and refresh), or otherwise prevent policy-bound sources from being materialized this way.
| } | ||
| // must be checked on both paths above: table resolution failing (which includes an | ||
| // authorization failure) falls back to a name-only lookup that binds nothing. | ||
| if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx, tableNameInfo.getCtl(), |
Contributor
There was a problem hiding this comment.
This authorizes only the named table, but dropping a primary key calls ConstraintManager.cascadeDropForeignKeys() and removes the actual FK constraints from every table in foreignTableInfos. A user with ALTER on the parent but none on a child can therefore delete the child's constraint. Require ALTER on every referencing table before the atomic mutation (or reject the cascade and require separately authorized FK drops), and cover the cross-table PK/FK case rather than only UNIQUE.
|
|
||
| // check auth. warming up moves data between compute groups, so require USAGE on both ends | ||
| // instead of global ADMIN. Keep it aligned with UseCloudClusterCommand. | ||
| checkComputeGroupUsage(connectContext, dstCluster); |
Contributor
There was a problem hiding this comment.
This USAGE gate admits non-admin ON TABLES jobs, but that mode then scans every internal DB/table and stores matching IDs/names without table authorization; refreshAllTableFilters() adds future matches without an effective identity to reauthorize them. A user with USAGE on both compute groups can therefore warm protected tables. Either keep pattern jobs ADMIN-only, or require SELECT for every initial match and persist the principal so future matches are reauthorized.
| if (table == null) { | ||
| ErrorReport.reportAnalysisException(ErrorCode.ERR_BAD_TABLE_ERROR, tableNameInfo.getTbl()); | ||
| } | ||
| if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(connectContext, tableNameInfo.getCtl(), |
Contributor
There was a problem hiding this comment.
The object resolved and later warmed is always internal.db.table, but this check uses the catalog supplied in SQL and the downstream triple drops the catalog. Thus SELECT on ext.db.t can authorize warming the same-named internal.db.t. The internal lookup and OlapTable cast also happen before this check, leaking object/type existence to denied users. Reject non-internal catalogs and authorize the internal fully qualified name before lookup/cast, or carry one resolved catalog/object consistently end to end.
…ternal warm up target Three follow-ups on the privilege checks: - DROP CONSTRAINT on a primary key cascades into ConstraintManager.cascadeDropForeignKeys(), which deletes the foreign key constraints of every referencing table. Require ALTER on those tables too. - WARM UP ... ON TABLES selects tables by pattern over the whole internal catalog and keeps re-matching in the background with no identity, so compute group USAGE is not enough to bound it. Require global ADMIN for that mode. - WARM UP ... WITH TABLE always resolves and warms the internal catalog, but the SELECT check used the catalog written in the SQL, so SELECT on ext_ctl.db.tbl authorized warming internal.db.tbl. Authorize the internal name, and do it before the db/table lookup. Also extend the constraint regression case with the cross-table PK/FK drop.
Member
Author
|
run buildall |
Member
Author
|
/review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
The Nereids path in
StmtExecutordispatches straight toCommand.run(), so each command has to enforce its own privileges. A number of commands never got that check, and can currently be executed by any authenticated user regardless of their grants.This PR adds the missing checks:
AdminSetEncryptionRootKeyCommand,AdminRotateTdeRootKeyCommandADMINAdminSetFrontendConfig,AdminSetTableStatus,AdminSetReplicaStatus,AdminCleanTrashDropCatalogRecycleBinCommandADMINSHOW CATALOG RECYCLE BINalready requires globalADMIN.RECOVERusesALTER_CREATE, but it is name-scoped while erasing takes a raw object id, so it cannot be authorized at db/table levelCreateDictionaryCommandCREATEon the dictionary andSELECTon the source tableCREATE TABLE/CREATE MTMV. TheSELECTcheck is needed because the load task runs internally, and unlike an MTMV the source table is not bound by the plannerDropDictionaryCommandDROPon the dictionaryDROP TABLE/DROP MTMVAddConstraintCommand,DropConstraintCommandALTERon the table. For a foreign key, also on the referenced table; for dropping a primary key, also on every referencing tableALTER TABLEWarmUpClusterCommandUSAGEon the source and destination compute groups, plusSELECTon each table named byWITH TABLE.ON TABLESadditionally requires globalADMINUseCloudClusterCommandCancelWarmUpJobCommandADMINCloudWarmUpJobrecords no owner, so a job cannot be scoped to the user who created itDropStageCommandADMINCreateStageCommand, which already checked itNote that
ADMIN_PRIVsatisfies every predicate used above, so admin users are unaffected by any of this.Where the check is placed, and why
AddConstraintCommand: for a foreign key the referenced table is checked as well, because adding the constraint registers a reverse reference on it.DropConstraintCommand: the check sits after the two table-resolution paths converge. Resolution can fall back to a name-only lookup, and putting the check on the normal path only would let the fallback skip it.DropConstraintCommand, primary keys: dropping one cascades intoConstraintManager.cascadeDropForeignKeys(), which deletes the foreign key constraint of every referencing table, soALTERis required on each of those too. The cascade is atomic, so all of them are checked beforedropConstraint().CreateDictionaryCommand: the check has to run aftervalidateAndSet(), since that is what fills in the default catalog/db names.WarmUpClusterCommand,WITH TABLE: the per-tableSELECTcheck is inside the existing resolution loop, before the db/table lookup. It authorizes the internal fully qualified name, not the catalog written in the SQL, because the lookup and the(db, table, partition)triple stored for the job are internal-catalog only.WarmUpClusterCommand,ON TABLES: this mode matches tables by glob over the whole internal catalog, andCacheHotspotManager.refreshAllTableFilters()keeps re-matching in the background with no identity available to re-authorize new matches. There is no fixed table set to authorize, so it requires globalADMINon top of the compute groupUSAGE.Incidental changes reviewers should look at
These are not privilege checks, but they fall out of adding them:
AdminRotateTdeRootKeyCommand,DropCatalogRecycleBinCommand,DropStageCommandhad novalidate()method at all. One was added to each and is called at the top ofrun().CreateDictionaryCommand.run()andDropDictionaryCommand.run()now declarethrows Exception.CreateDictionaryCommand.run(): the single try block that wrappedvalidateAndSet()+createDictionary()is split in two, with the check in between. Consequence: an access-denied error is not wrapped in the"Failed to create dictionary: ..."prefix, while the failure messages fromvalidateAndSet()andcreateDictionary()are unchanged.WarmUpClusterCommand.validate(): order is now cloud-mode → compute groupUSAGE(+ADMINforON TABLES) → compute group existence/virtual-group validation → table resolution. The non-cloud error message is unchanged, but in cloud mode a user withoutUSAGEnow gets an access-denied error where they previously got "compute group doesn't exist".WarmUpClusterCommand,WITH TABLE: theSELECTcheck runs before the db/table lookup, so a user withoutSELECTgets an access-denied error where they previously got "unknown database/table". This is deliberate: the error should not tell a user who cannot read the table whether it exists.DropConstraintCommand: the twoALTERchecks are factored into a privatecheckAlterPriv(), same shape as the one inAddConstraintCommand.Open questions
WarmUpClusterCommand: forWITH TABLE, globalADMINfelt too coarse for an operation scoped to compute groups the user already hasUSAGEon, so it usescheckCloudPriv(..., ResourceTypeEnum.CLUSTER)plus per-tableSELECT. Happy to switch it back toADMINif the cloud maintainers prefer that.ON TABLESdoes requireADMIN, since a pattern job cannot be authorized per table.DropConstraintCommand: requiringALTERon the referencing tables means a user who owns the primary key table can no longer drop it once somebody else's table references it. Rejecting the cascade and asking for the foreign keys to be dropped separately would be the other option; happy to switch.CancelWarmUpJobCommandkeepsADMINonly because there is nothing to scope it to. IfCloudWarmUpJobrecorded the submitting user, letting that user cancel their own job would be better.ShowWarmUpCommand(SHOW WARM UP JOB) andShowDictionariesCommand(SHOW DICTIONARIES) have no privilege check either. Happy to follow up in a separate PR.Release note
Fix missing privilege checks on
ADMIN SET ENCRYPTION ROOT KEY,ADMIN ROTATE TDE ROOT KEY,DROP CATALOG RECYCLE BIN,CREATE/DROP DICTIONARY,ALTER TABLE ADD/DROP CONSTRAINT,WARM UP CLUSTER,CANCEL WARM UP JOBandDROP STAGE. These statements previously ran for any authenticated user.Check List (For Author)
New
auth_callcases for constraint, dictionary and recycle bin, each covering both the denied and the granted path, including the cross-table primary key / foreign key cascade. The cloud-only commands (WARM UP CLUSTER,CANCEL WARM UP JOB,DROP STAGE) are not covered by a regression case.Behavior changed:
Does this need documentation?
The privilege documentation for these statements should list the required privileges.